[VPA] Add subresource status for vpa with e2e fix #5766

wu0407 · 2023-05-17T15:03:08Z

Add status field in subresource on crd yaml and add new ClusterRole system:vpa-actor to patch /status subresource.
The metadata.generation only increase on vpa spec update.
Fix e2e test for patch and create vpa

What type of PR is this?

/kind bug
/kind failing-test
/kind api-change

What this PR does / why we need it:

Current vpa crd has empty subresource field, that leads to metadata.generation increase on vpa status update.
The controller-runtime has GenerationChangedPredicate to filter out update status event for cr, but it not work for vpa.
This PR add status field in subresource on crd yaml and add new ClusterRole system:vpa-actor to patch /status subresource.
The metadata.generation only increase on vpa spec update.

Which issue(s) this PR fixes:

Fixes #5675

Special notes for your reviewer:

Previous pull request is #5680 and be revert #5738, because of e2e test failed #5727.
This PR fixes e2e test failure.

Does this PR introduce a user-facing change?

Added the /status subresource to VPA
action required: If you're installing VPA with different means than the `hack/vpa-up.sh` script, you need to add the ClusterRole `system:vpa-status-actor` and corresponding ClusterRoleBinding yourself. Otherwise VPA will no longer be able to update the recommendations!
If you create vpa with status,  status be ignored in apiserver, so need to update status after create vpa.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

wu0407 · 2023-05-17T15:06:09Z

@voelzmo @jbartosik

k8s-triage-robot · 2023-05-17T15:24:10Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

jbartosik

We should mention the breaking change in the release notes.

Also a couple comments asking for some small code changes.

vertical-pod-autoscaler/e2e/v1/common.go

vertical-pod-autoscaler/e2e/v1beta2/common.go

Add status field in subresource on crd yaml and add new ClusterRole system:vpa-actor to patch /status subresource. The `metadata.generation` only increase on vpa spec update. Fix e2e test for patch and create vpa

wu0407 · 2023-05-19T05:21:09Z

vertical-pod-autoscaler/e2e/v1beta2/common.go

+	// apiserver ignore status in vpa create, so need to update status
+	if !isStatusEmpty(&vpa.Status) {
+		if vpa.Status.Recommendation != nil {
+			PatchVpaRecommendation(f, vpa, vpa.Status.Recommendation)
+		}
+	}


A comment is needed in InstallVPA that create vpa with status need individual update status, so isStatusEmpty needs to be preserved, and the code may appear a bit redundant

wu0407 · 2023-05-19T05:24:07Z

A comment is needed leave in InstallVPA that create vpa with status need individual update status, so isStatusEmpty needs to be preserved, and the code may appear a bit redundant

We should mention the breaking change in the release notes.

Also a couple comments asking for some small code changes.

A note in release notes is done

jbartosik

Looks good to me.

I left one small comment to make code a bit cleaner.

I think this is good to merge but:

Our release is overdue, I want to wait with merging until we release to avoid further delays
I want to check if we're good on the API change (this is a breaking API change)

So
/hold

jbartosik · 2023-06-14T07:43:43Z

vertical-pod-autoscaler/e2e/v1/common.go

@@ -372,6 +372,23 @@ func InstallVPA(f *framework.Framework, vpa *vpa_types.VerticalPodAutoscaler) {
 	vpaClientSet := getVpaClientSet(f)
 	_, err := vpaClientSet.AutoscalingV1().VerticalPodAutoscalers(f.Namespace.Name).Create(context.TODO(), vpa, metav1.CreateOptions{})
 	gomega.Expect(err).NotTo(gomega.HaveOccurred(), "unexpected error creating VPA")
+	// apiserver ignore status in vpa create, so need to update status
+	if !isStatusEmpty(&vpa.Status) {
+		if vpa.Status.Recommendation != nil {


I'd write this as one condition:

if !isStatusEmpty(&vpa.Status) && vpa.Status.Recommendation != nil {

jbartosik · 2023-06-14T12:24:46Z

Release is tracked in #5851

kwiesmueller · 2023-06-15T20:44:50Z

vertical-pod-autoscaler/pkg/utils/vpa/api.go

 	bytes, err := json.Marshal(patches)
 	if err != nil {
 		klog.Errorf("Cannot marshal VPA status patches %+v. Reason: %+v", patches, err)
 		return
 	}

-	return vpaClient.Patch(context.TODO(), vpaName, types.JSONPatchType, bytes, meta.PatchOptions{})
+	return vpaClient.Patch(context.TODO(), vpaName, types.JSONPatchType, bytes, meta.PatchOptions{}, "status")


Is there a unit test we can add to cover this change?
Maybe using the fake client?

It is already have test case on TestUpdateVpaIfNeeded

autoscaler/vertical-pod-autoscaler/pkg/utils/vpa/api_test.go

Lines 93 to 101 in 1384c8b

for _, tc := range testCases {

t.Run(tc.caseName, func(t *testing.T) {

fakeClient := vpa_fake.NewSimpleClientset(&vpa_types.VerticalPodAutoscalerList{Items: []vpa_types.VerticalPodAutoscaler{*tc.observedVpa}})

_, err := UpdateVpaStatusIfNeeded(fakeClient.AutoscalingV1().VerticalPodAutoscalers(tc.updatedVpa.Namespace),

tc.updatedVpa.Name, &tc.updatedVpa.Status, &tc.observedVpa.Status)

assert.NoError(t, err, "Unexpected error occurred.")

actions := fakeClient.Actions()

if tc.expectedUpdate {

assert.Equal(t, 1, len(actions), "Unexpected number of actions")

Thanks, makes sense.

kwiesmueller · 2023-06-21T15:29:28Z

/lgtm
/hold

kwiesmueller · 2023-06-26T20:10:39Z

The release is published, are we good to unhold @jbartosik ?

jbartosik

/lgtm
/approve
/hold cancel

Yup, I think we can merge this

k8s-ci-robot · 2023-06-27T08:48:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jbartosik, wu0407

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~vertical-pod-autoscaler/OWNERS~~ [jbartosik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kwiesmueller · 2023-06-28T14:34:28Z

The PR was reverted in #5897
I'll try to run the e2e test locally to reproduce. @wu0407 please reach out if I can help with getting this back in.
You can also reach me on the kubernetes slack (kwiesmueller).

wu0407 · 2023-06-29T02:59:33Z

The PR was reverted in #5897 I'll try to run the e2e test locally to reproduce. @wu0407 please reach out if I can help with getting this back in. You can also reach me on the kubernetes slack (kwiesmueller).

please help me point what is the problem, because i has not enviroment to test. I get a acount on kubernetes slack. @kwiesmueller

k8s-ci-robot requested review from jbartosik and krzysied May 17, 2023 15:03

jbartosik requested changes May 18, 2023

View reviewed changes

vertical-pod-autoscaler/e2e/v1/common.go Outdated Show resolved Hide resolved

vertical-pod-autoscaler/e2e/v1beta2/common.go Show resolved Hide resolved

Add subresource status for vpa

1384c8b

Add status field in subresource on crd yaml and add new ClusterRole system:vpa-actor to patch /status subresource. The `metadata.generation` only increase on vpa spec update. Fix e2e test for patch and create vpa

wu0407 force-pushed the add-status-subresource branch from 9a0521a to 1384c8b Compare May 19, 2023 05:10

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 19, 2023

wu0407 commented May 19, 2023

View reviewed changes

jbartosik approved these changes Jun 14, 2023

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 14, 2023

kwiesmueller reviewed Jun 15, 2023

View reviewed changes

jbartosik mentioned this pull request Jun 21, 2023

External Metrics support for VPA recommender #5576

Merged

k8s-ci-robot assigned kwiesmueller Jun 21, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2023

jbartosik added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 27, 2023

jbartosik approved these changes Jun 27, 2023

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2023

k8s-ci-robot assigned jbartosik Jun 27, 2023

k8s-ci-robot merged commit 2aaa41d into kubernetes:master Jun 27, 2023

jbartosik mentioned this pull request Jun 28, 2023

Revert "Add subresource status for vpa" #5897

Merged

wu0407 mentioned this pull request Jun 29, 2023

Add subresource status for vpa #5911

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VPA] Add subresource status for vpa with e2e fix #5766

[VPA] Add subresource status for vpa with e2e fix #5766

wu0407 commented May 17, 2023 •

edited

Loading

wu0407 commented May 17, 2023

k8s-triage-robot commented May 17, 2023

jbartosik left a comment

wu0407 May 19, 2023 •

edited

Loading

wu0407 commented May 19, 2023

jbartosik left a comment

jbartosik Jun 14, 2023

jbartosik commented Jun 14, 2023

kwiesmueller Jun 15, 2023

wu0407 Jun 16, 2023

kwiesmueller Jun 21, 2023

kwiesmueller commented Jun 21, 2023

kwiesmueller commented Jun 26, 2023

jbartosik left a comment

k8s-ci-robot commented Jun 27, 2023

kwiesmueller commented Jun 28, 2023

wu0407 commented Jun 29, 2023 •

edited

Loading

	for _, tc := range testCases {
	t.Run(tc.caseName, func(t *testing.T) {
	fakeClient := vpa_fake.NewSimpleClientset(&vpa_types.VerticalPodAutoscalerList{Items: []vpa_types.VerticalPodAutoscaler{*tc.observedVpa}})
	_, err := UpdateVpaStatusIfNeeded(fakeClient.AutoscalingV1().VerticalPodAutoscalers(tc.updatedVpa.Namespace),
	tc.updatedVpa.Name, &tc.updatedVpa.Status, &tc.observedVpa.Status)
	assert.NoError(t, err, "Unexpected error occurred.")
	actions := fakeClient.Actions()
	if tc.expectedUpdate {
	assert.Equal(t, 1, len(actions), "Unexpected number of actions")

[VPA] Add subresource status for vpa with e2e fix #5766

[VPA] Add subresource status for vpa with e2e fix #5766

Conversation

wu0407 commented May 17, 2023 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

wu0407 commented May 17, 2023

k8s-triage-robot commented May 17, 2023

jbartosik left a comment

Choose a reason for hiding this comment

wu0407 May 19, 2023 • edited Loading

Choose a reason for hiding this comment

wu0407 commented May 19, 2023

jbartosik left a comment

Choose a reason for hiding this comment

jbartosik Jun 14, 2023

Choose a reason for hiding this comment

jbartosik commented Jun 14, 2023

kwiesmueller Jun 15, 2023

Choose a reason for hiding this comment

wu0407 Jun 16, 2023

Choose a reason for hiding this comment

kwiesmueller Jun 21, 2023

Choose a reason for hiding this comment

kwiesmueller commented Jun 21, 2023

kwiesmueller commented Jun 26, 2023

jbartosik left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 27, 2023

kwiesmueller commented Jun 28, 2023

wu0407 commented Jun 29, 2023 • edited Loading

wu0407 commented May 17, 2023 •

edited

Loading

wu0407 May 19, 2023 •

edited

Loading

wu0407 commented Jun 29, 2023 •

edited

Loading